1 task by nrsxx · Pull Request #346 · fediq/mipt-java-2016

nrsxx · 2017-03-01T22:46:25Z

Second attempt:)

ignorer · 2017-03-04T08:51:13Z

Какой знакомый код. А Елена Смирнова точно дала своё согласие на его заимствование?

DanAnastasyev · 2017-03-12T17:28:42Z

...work-g596-narsia/src/main/java/ru/mipt/java2016/homework/g596/narsia/task1/MyCalculator.java

-    public double calculate(String Expression) throws ParsingException {
-        if (!isExpressionCorrect(Expression)) {
-            throw new ParsingException("Invalid expression");
+    private String whatIsIt(Character Symbol) {


"Ochen` ploxo", ochen'
Неужели нельзя это не так по-индуски сделать?

ignorer

Вынужден сообщить, что читаемость у этого кода далеко не на самом высоком уровне. Прочитав два десятка калькуляторов, я не могу понять, как работает твой, хотя идеологически он вряд ли отличается сильно. Проблема на мой взгляд в слишком больших switch блоках, злоупотреблении ими и отсутствии хоть каких-то комментариев к происходящему.

Некоторые замечания, которые я оставил к твоему коду, относятся не только к той строчке, к которой привязаны, поэтому будь добр, пройдись по всему коду и исправь ошибки в аналогичных местах.

ignorer · 2017-03-14T10:12:06Z

...work-g596-narsia/src/main/java/ru/mipt/java2016/homework/g596/narsia/task1/MyCalculator.java

-    public double calculate(String Expression) throws ParsingException {
-        if (!isExpressionCorrect(Expression)) {
-            throw new ParsingException("Invalid expression");
+    private String whatIsIt(Character symbol) {


Любой человек, который знает Java, понимает разницу между базовыми типами и их обёртками, а также про боксинг и анбоксинг, про который семинарист рассказывал вам ещё в начале семестра. Настоятельно рекомендую почитать материалы семинаров, документацию по java или stackoverflow.com и ознакомиться с тем, что такое primitive wrapper class, boxing и unboxing, а после этого со знанием дела объяснить, зачем тебе здесь именно Character.

ignorer · 2017-03-14T10:20:49Z

...work-g596-narsia/src/main/java/ru/mipt/java2016/homework/g596/narsia/task1/MyCalculator.java

-    public double calculate(String Expression) throws ParsingException {
-        if (!isExpressionCorrect(Expression)) {
-            throw new ParsingException("Invalid expression");
+    private String whatIsIt(Character symbol) {


Ты можешь представить, какой оверхед создаёт эта функция? Если Java ничего не оптимизирует, а будет под каждый вызов твоей волшебной функции whatIsIT() создавать новую строку, этот калькулятор будет съедать памяти раз в 50 больше нужного.
Теперь вопрос: а почему, собственно, возвращается строка? enum SymbolType в списанном решении был таким хорошим, а тут у тебя аж строки возвращаются. Мне кажется, что ответ - чтобы код не был похож на предыдущее решение, но это не точно.

ignorer · 2017-03-14T10:27:46Z

...work-g596-narsia/src/main/java/ru/mipt/java2016/homework/g596/narsia/task1/MyCalculator.java

+            case '6':
+            case '7':
+            case '8':
+            case '9':


Итак, у нас есть задача - нужно определить, является ли символ цифрой. У этой, не побоюсь этого слова, задачи есть несколько решений, из которых ты выбрал самое беспонтовое. Предлагаю ознакомиться с документацией к классу Character (хотя бы пролистать список методов) и сказать, что нам предлагает сам язык в этом плане.

ignorer · 2017-03-14T10:31:58Z

...work-g596-narsia/src/main/java/ru/mipt/java2016/homework/g596/narsia/task1/MyCalculator.java

+            }
+            case ' ':
+            case '\t':
+            case '\n':


Аналогично тому, что я писал выше про числа - для этого уже сделана стандартная функция, и тебе предлагается её отыскать.

ignorer · 2017-03-14T10:33:48Z

...work-g596-narsia/src/main/java/ru/mipt/java2016/homework/g596/narsia/task1/MyCalculator.java

-    private enum SymbolType {
-        OPENING_BRACKET, CLOSING_BRACKET, NUMBER, OPERATOR, NOTHING
+
+    private Integer getCode(Character first, Character second) {


А какой именно код возвращает эта функция? Лично мне сходу вообще непонятна семантика этой функции, в которой к тому же ещё и вложенные switch блоки, а значения кодов - тайна, покрытая мраком и нигде не задокументированная.

ignorer · 2017-03-14T10:42:19Z

...work-g596-narsia/src/main/java/ru/mipt/java2016/homework/g596/narsia/task1/MyCalculator.java

-                    if (previousSymbol == SymbolType.OPENING_BRACKET ||
-                            previousSymbol == SymbolType.OPERATOR) {
-                        return false; //открывающая-закрывающая, оператор-закрывающая
+        for (int cnt = 0; cnt < expression.length(); ++cnt) {


Название cnt по меньшей мере чрезвычайно странное. Как минимум оно трактуется неоднозначно - у тебя оно ассоциируется со словом count, у меня со словом cunt (или наоборот), а двузначность в коде - плохо. И вообще count - это количество или подсчёт, но никак не индекс (счётчик здесь тоже не подходит, ты же ничего не считаешь, а только итерируешься по строке).
И привыкай не использовать в коде сокращения имён (кроме общепринятых). Они хороши только когда пишешь код на листочке без автодополнения.

ignorer · 2017-03-14T11:00:05Z

...work-g596-narsia/src/main/java/ru/mipt/java2016/homework/g596/narsia/task1/MyCalculator.java

+    private StringBuilder removeSpaces(String expression) {
+        StringBuilder result = new StringBuilder(expression.length());
+        for (int cnt = 0; cnt < expression.length(); ++cnt) {
+            if (!whatIsIt(expression.charAt(cnt)).equals("Space")) {


Вот это можно вообще без whatIsIt() сделать также одной строчкой.

ignorer · 2017-03-14T11:01:59Z

...work-g596-narsia/src/main/java/ru/mipt/java2016/homework/g596/narsia/task1/MyCalculator.java

+        expression.append('~');
+        Character curSymbol;
+        Character prevSymbol = '~';
+        Stack<Character> Texas = new Stack<>();


В java code conventions явно указывается, что переменные называются с маленькой буквы.

ignorer · 2017-03-14T11:02:48Z

...work-g596-narsia/src/main/java/ru/mipt/java2016/homework/g596/narsia/task1/MyCalculator.java

-            if (currChar == ' ' || currChar == '\n' || currChar == '\t') {
-                continue;
+        Integer cnt = 0;
+        while (true) {


Как интересно ты в исходном решении заменил for на while.

ignorer · 2017-03-14T11:05:13Z

...work-g596-narsia/src/main/java/ru/mipt/java2016/homework/g596/narsia/task1/MyCalculator.java

+    @Override
+    public double calculate(String expression) throws ParsingException {
+        isAlmostValid(expression);
+        StringBuilder withoutSpaces = removeSpaces(expression);


А зачем тебе именно StringBuilder возвращать из функции? Ты же можешь вернуть return stringBuilder.toString(), и тогда на выходе будет строка, и тип переменной withoutSpaces будет вполне себе понятным - String. А то пока это выглядит так, как будто ты планируешь дальше строить эту строку.

DanAnastasyev · 2017-03-15T18:44:55Z

...work-g596-narsia/src/main/java/ru/mipt/java2016/homework/g596/narsia/task1/MyCalculator.java

+import ru.mipt.java2016.homework.base.task1.Calculator;
+import ru.mipt.java2016.homework.base.task1.ParsingException;
+
+import java.util.*;


Считается плохим тоном импортировать через *. Перечисли всё, что ты импортируешь отдельно

DanAnastasyev · 2017-03-15T18:49:15Z

...work-g596-narsia/src/main/java/ru/mipt/java2016/homework/g596/narsia/task1/MyCalculator.java

+import java.util.*;
+
+// подробное описание работы кода
+// можно найти в файле description


Пиши комментарии прямо в коде

DanAnastasyev · 2017-03-15T18:51:14Z

...work-g596-narsia/src/main/java/ru/mipt/java2016/homework/g596/narsia/task1/MyCalculator.java

+        if (symbol.equals('~')) {
+            return SymbolType.FIRST;
+        }
+        return SymbolType.INVALID;


Во многих языках программирования есть такая штука, как assert. Его используют, чтобы поймать некорректные состояния. Этот твой SymbolType.INVALID - как раз такое состояние. Кидай отсюда исключение

DanAnastasyev · 2017-03-15T18:52:30Z

...work-g596-narsia/src/main/java/ru/mipt/java2016/homework/g596/narsia/task1/MyCalculator.java

+                            OPENING_BRACKET, CLOSING_BRACKET, FIRST, INVALID }
+
+
+    private SymbolType whatIsIt(Character symbol) {


Не слишком-то похоже на понятное название
Почему принимает Character?

DanAnastasyev · 2017-03-15T18:56:19Z

...work-g596-narsia/src/main/java/ru/mipt/java2016/homework/g596/narsia/task1/MyCalculator.java

+    }
+
+
+    private int getCode(char first, char second) {


Как говорили Великие:

А какой именно код возвращает эта функция? Лично мне сходу вообще непонятна семантика этой функции, в которой к тому же ещё и вложенные switch блоки, а значения кодов - тайна, покрытая мраком и нигде не задокументированная.

Почему не сделать enum вместо цифр? С ним назначение функции слегка прояснится. Заодно можно будет переименовать getCode во что-нибудь более определенное

DanAnastasyev · 2017-03-15T18:57:35Z

...work-g596-narsia/src/main/java/ru/mipt/java2016/homework/g596/narsia/task1/MyCalculator.java

+        Character curSymbol;
+        Character prevSymbol = '~';
+        Stack<Character> texas = new Stack<>();
+        StringBuilder california = new StringBuilder(expression.length());


То, что в статье, которую ты скинул, есть слова Техас и Калифорния, ещё не значит, что их нужно использовать, как названия переменных. Придумай что-нибудь более говорящее

1 task

d449e5f

Second attempt

605a5a4

DanAnastasyev reviewed Mar 12, 2017

View reviewed changes

String -> StringBuilder

ed20414

ignorer requested changes Mar 14, 2017

View reviewed changes

nrsxx added 3 commits March 15, 2017 06:03

Fixes in total + description file added

251fdf2

Codestyle

820b723

Codestyle last time

1f82f19

DanAnastasyev reviewed Mar 15, 2017

View reviewed changes

nrsxx added 2 commits March 16, 2017 21:23

Fixes

a152601

Codestyle fixes

6069427

ignorer approved these changes Mar 21, 2017

View reviewed changes

		OPENING_BRACKET, CLOSING_BRACKET, FIRST, INVALID }


		private SymbolType whatIsIt(Character symbol) {

Conversation

nrsxx commented Mar 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ignorer commented Mar 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ignorer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nrsxx commented Mar 1, 2017 •

edited

Loading